I'd like to avoid compound property names, if possible. How do you feel about borrowing from HTML here and just use src, instead of imageUrl and href, instead of hyperlinkUrl?

I wanted to keep the parity with what we receive from Davinci, but totally open to using simpler names. This is what we receive from Davinci

   "type": "IMAGE",
   "key": "<field key>",
   "description": "<alt text>",
   "imageUrl": "<absolute URL>",
   "hyperlinkUrl": "<absolute URL>"

Do you have a preference there? I feel it's easier to reason through and debug when we keep the same properties as what Davinci returns. Besides, I checked and almost all collectors except for QrCodeField follow property names the same as what Davinci returns.

I can change it to alt, src, href. However, would be good to know your opinion on whether we should keep that parity or borrow from HTML.

I thought about this further. davinci.types.ts already maps what Davinci returns, so we can rename those properties independently from Davinci in collector.types.ts file. Updated the description, imageUrl, hyperlinkUrl properties to alt, src, href respectively.

What do you think about copying field.description here into label so it's not empty?

I see what you mean, something like field.content ?? field.description ?? ''

Among the NoValueCollectors, description is used only by ImageCollector. We can put that as a label, but it seems like we're adding an image collector specific property to a generic NoValueCollector.

However, if we were to extend no value collector for other fields later and if we find that description is an alternative property to content, maybe we can add it there then.

Right now, I feel like it might cause future bugs where NoValueCollector now checks for description if content is null, which is something specific to only image collector, and description may not be a property or description might mean something else for other no value collectors that we add later.

What if we only override the label property in the returnImageCollector function?

Yeah, that sounds interesting. I'm thinking this will change the naming for alt to label. I do like the idea that we can reuse an existing field but at the same time if we prefer HTML specific names like src, alt, href, like Justin mentioned in the earlier comment, then it kind of deviates in terms of naming.

It doesn't have to change the alt naming necessarily. The value of field.description could be placed into both alt and label. We do something similar in NoValueCollectors where we sometimes put field.content into both output.label and output.content. I think having some value for label instead of leaving it blank keeps it consistent with other collectors from this category and avoids having a "dead" property.

Ah, okay. I can do that and continue to use alt. That is perfect, if that is what you are referring to. Just pushed the commit!

🗄️ Data Integrity & Integration | 🟡 Minor

#!/bin/bash
# Description: Compare error handling across collector factory functions

# Check if other collectors override the error property
rg -A 10 'export function return.*Collector' packages/davinci-client/src/lib/collector.utils.ts | rg -C 2 'error:'

#!/bin/bash
# Read the area around line 1004 to identify which collector this is
sed -n '990,1020p' packages/davinci-client/src/lib/collector.utils.ts

#!/bin/bash
# Search for returnQrCodeCollector and returnReadOnlyCollector functions and their error handling
rg -B 5 -A 15 'export function returnQrCodeCollector|export function returnReadOnlyCollector' packages/davinci-client/src/lib/collector.utils.ts

#!/bin/bash
# Get a complete view of all collector factory functions and their error property handling
ast-grep outline packages/davinci-client/src/lib/collector.utils.ts | head -50

#!/bin/bash
# Find and read the returnNoValueCollector implementation
sed -n '905,939p' packages/davinci-client/src/lib/collector.utils.ts

Remove explicit error: null override or document why IMAGE collectors bypass validation errors.

returnImageCollector explicitly sets error: null, overriding validation from the base returnNoValueCollector. In contrast, returnQrCodeCollector and returnReadOnlyCollector inherit the base error state, which validates that content and type fields exist.

Either IMAGE collectors should inherit the base validation (removing the explicit null), or this difference should be documented with a comment explaining why IMAGE collectors skip error validation.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/davinci-client/src/lib/collector.utils.ts` at line 1004, The
returnImageCollector function explicitly sets error: null, which overrides
validation errors from the base returnNoValueCollector, creating inconsistency
with other collectors like returnQrCodeCollector and returnReadOnlyCollector
that inherit the base error state. Either remove the explicit error: null
assignment from returnImageCollector to inherit the base validation behavior
like the other collectors do, or keep it and add a clear comment documenting why
IMAGE collectors specifically need to bypass validation error checking while
other collectors do not.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Consider adding fallback for src to handle missing imageUrl.

The src property is set directly from field.imageUrl without a fallback. If imageUrl is null or undefined, the resulting image element will have an invalid src attribute. For comparison, returnQrCodeCollector (line 983) uses field.content || '' to provide a fallback for its src property.

      label: field.description ?? '',
-      src: field.imageUrl,
+      src: field.imageUrl ?? '',
      alt: field.description,

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/davinci-client/src/lib/collector.utils.ts` at line 1008, The `src`
property assignment using `field.imageUrl` lacks a fallback mechanism for when
imageUrl is null or undefined, resulting in invalid src attributes. Add a
fallback value to the `src: field.imageUrl` assignment by using the pattern
`src: field.imageUrl || ''` to match the approach used in the
`returnQrCodeCollector` function, ensuring the image element always has a valid
(even if empty) src attribute.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Add fallback for alt to match label handling.

The alt property uses field.description directly without a fallback, while label on line 1007 uses field.description ?? ''. This inconsistency means that when field.description is absent, label will be an empty string but alt will be undefined. Based on the unit tests (which verify label defaults to '' when description is missing), the same defensive handling should apply to alt.

      label: field.description ?? '',
      src: field.imageUrl,
-      alt: field.description,
+      alt: field.description ?? '',
      ...(field.hyperlinkUrl ? { href: field.hyperlinkUrl } : {}),

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/davinci-client/src/lib/collector.utils.ts` at line 1009, The `alt`
property assignment on line 1009 uses `field.description` directly without a
fallback value, while the `label` property on line 1007 uses `field.description
?? ''` to provide an empty string default. This inconsistency means `alt` will
be `undefined` when `field.description` is missing, while `label` will be an
empty string. Apply the same defensive null coalescing pattern to the `alt`
assignment to use an empty string fallback when `field.description` is absent,
ensuring consistent behavior between both properties.